-
Notifications
You must be signed in to change notification settings - Fork 266
hc-dpki cli for DeepKey #2117
base: develop
Are you sure you want to change the base?
hc-dpki cli for DeepKey #2117
Conversation
|
@zo-el I notice a bunch of tests are breaking, would like to review this after tests pass... |
* updated tests
|
@zippy this PR is ready for review |
| quiet, | ||
| ); | ||
| keygen_standalone(keystore_passphrase)? | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a few panics in here even though we are returning an HCResult, is this because we cannot recover from them if they happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, these expects should be replaced with ?
| .read_line(&mut input) | ||
| .expect("Could not read from stdin"); | ||
| match input.as_str() { | ||
| "Y\n" => true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we care about lowercase/uppercase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not Really
| &mut self.inner.buf, | ||
| )?; | ||
| Ok(KeyBundle::new_from_seed_buf(&mut ref_seed_buf)?) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably generalize these two structs with an enum.
enum Seed
{ Auth,Revocation}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but had a few questions about the number of expects we have in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @AshantiMutinta's comments
| quiet, | ||
| ); | ||
| keygen_standalone(keystore_passphrase)? | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, these expects should be replaced with ?
PR summary
Adds some new commands to the CLI tool for managing DPKI keys.
hc dpki genrootcreates a new random root seedhc dpki keygenis similar tohc keygenbut will use a dpki root seed mnemonic so that keys can be recoveredhc dpki genrevoke/genauthproduce revocation/auth seeds from which keys can be derived.hc dpki signuse a revocation/auth seed to produce a key then sign a message (which can be an agent key). This is the process by which keys can be authorized and revokedEach time a mnemonic is produced it is optionally encrypted with a passphrase
testing/benchmarking notes
( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )
followups
( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )
changelog
documentation